ART-21775: add doozer beta:release-payload:rebase-and-build command - #3218
ART-21775: add doozer beta:release-payload:rebase-and-build command#3218ashwindasr wants to merge 3 commits into
Conversation
|
Skipping CI for Draft Pull Request. |
|
@ashwindasr: This pull request references ART-21775 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
openshift-ci-robot
added
the
jira/valid-reference
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughChangesThe PR adds a CLI that generates OpenShift release payload sources, commits them to a repository, and optionally starts a multi-architecture Konflux build. It also adds command wiring, configuration constants, error handling, and tests. Release payload workflow
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant ReleasePayloadRebaseAndBuildCli
participant OpenShiftCLI
participant GitRepository
participant Konflux
User->>ReleasePayloadRebaseAndBuildCli: invoke release_payload_rebase_and_build
ReleasePayloadRebaseAndBuildCli->>OpenShiftCLI: generate and validate release manifests
OpenShiftCLI-->>ReleasePayloadRebaseAndBuildCli: return image-references and CVO pullspec
ReleasePayloadRebaseAndBuildCli->>GitRepository: write and commit payload sources
GitRepository-->>ReleasePayloadRebaseAndBuildCli: return commit hash
ReleasePayloadRebaseAndBuildCli->>Konflux: start multi-architecture PipelineRun
Konflux-->>ReleasePayloadRebaseAndBuildCli: return build outcome
ReleasePayloadRebaseAndBuildCli-->>User: emit JSON or human-readable result
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (9 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (6)
doozer/doozerlib/cli/release_payload.py (3)
207-208: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer a domain exception over
IOErrorfor this precondition.The missing commit is a programming or workflow error, not an I/O failure.
DoozerFatalErroris already imported and is used for the other fatal conditions in this file. A change also requires an update totest_build_raises_without_commitindoozer/tests/cli/test_release_payload.py.♻️ Proposed change
if not build_repo.commit_hash: - raise IOError("Release payload repository must have a commit to build. Did you rebase?") + raise DoozerFatalError("Release payload repository must have a commit to build. Did you rebase?")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@doozer/doozerlib/cli/release_payload.py` around lines 207 - 208, Replace the IOError raised by the missing-commit precondition in the release payload build flow with the already imported DoozerFatalError, preserving the existing message. Update test_build_raises_without_commit to expect DoozerFatalError instead of IOError.
273-273: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReplace the
assertwith an explicit check.Python removes
assertstatements when the interpreter runs with-O. Ifgroup_configisNone, the failure then moves to a laterAttributeError. Raise an explicit error instead.♻️ Proposed change
- assert runtime.group_config is not None, "group_config is not loaded; Doozer bug?" + if runtime.group_config is None: + raise DoozerFatalError("group_config is not loaded; Doozer bug?")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@doozer/doozerlib/cli/release_payload.py` at line 273, Replace the assert guarding runtime.group_config with an explicit None check that raises an appropriate error using the existing diagnostic message, ensuring the failure occurs even when Python runs with optimizations.
485-491: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLog the exception before you exit for JSON output.
The
--output jsonbranch replaces the exception withstr(e)and exits. The traceback is then lost. Log the exception first so failures stay diagnosable in CI.Note: the static analysis hint that recommends
jsonifytargets Flask responses. It does not apply to this Click command.♻️ Proposed change
except Exception as e: if output == 'json': + LOGGER.exception("Release payload rebase and build failed") click.echo(json.dumps({"error": str(e)}, indent=2)) sys.exit(1) raise🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@doozer/doozerlib/cli/release_payload.py` around lines 485 - 491, Update the exception handler around cli_obj.run so the JSON-output branch logs the caught exception, including traceback details, before emitting the JSON error and exiting. Preserve the existing non-JSON behavior that re-raises the exception, and do not replace Click output handling with Flask-specific jsonify.Source: Linters/SAST tools
doozer/tests/cli/test_release_payload.py (3)
144-146: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStrengthen the negative assertion.
assertNotIn("--from-image-stream=4.21-konflux-art-latest", cmd)passes even if the command contains a different--from-image-streamvalue. Assert that no--from-image-streamargument exists at all.♻️ Proposed change
cmd = mock_cmd_assert_async.call_args.args[0] self.assertIn("--from-release=registry.example.com/ocp/release:4.21.0", cmd) - self.assertNotIn("--from-image-stream=4.21-konflux-art-latest", cmd) + self.assertFalse([arg for arg in cmd if str(arg).startswith("--from-image-stream")]) + self.assertNotIn("--reference-mode=source", cmd)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@doozer/tests/cli/test_release_payload.py` around lines 144 - 146, Strengthen the assertions around cmd in the release payload test by verifying that no argument with the --from-image-stream option exists, regardless of its value; retain the existing assertion for the expected --from-release argument.
340-340: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAvoid the hardcoded
/tmppath in the mock attribute.Ruff reports S108 on this line. The value is only used in a log message, so a neutral placeholder removes the finding without changing test behavior.
♻️ Proposed change
- self.build_repo.local_dir = "/tmp/release-payload" + self.build_repo.local_dir = Path(tempfile.gettempdir(), "release-payload")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@doozer/tests/cli/test_release_payload.py` at line 340, Replace the hardcoded /tmp/release-payload value assigned to self.build_repo.local_dir with a neutral non-filesystem placeholder, preserving its use in the log message and the test’s behavior.Source: Linters/SAST tools
343-442: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider adding a test for the Click command wrapper.
The tests cover
ReleasePayloadRebaseAndBuildCliwell. They do not coverrelease_payload_rebase_and_build. ACliRunnertest would confirm theKONFLUX_SA_KUBECONFIGfallback, the--output jsonpayload, and the exit code 1 on failure.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@doozer/tests/cli/test_release_payload.py` around lines 343 - 442, Add a CliRunner-based test for the release_payload_rebase_and_build command wrapper, covering KONFLUX_SA_KUBECONFIG fallback behavior, JSON output via --output json, and exit code 1 when the command fails. Reuse the existing ReleasePayloadRebaseAndBuildCli test fixtures and mock the underlying execution so the test verifies wrapper behavior without performing real rebases or builds.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@doozer/doozerlib/cli/release_payload.py`:
- Around line 207-208: Replace the IOError raised by the missing-commit
precondition in the release payload build flow with the already imported
DoozerFatalError, preserving the existing message. Update
test_build_raises_without_commit to expect DoozerFatalError instead of IOError.
- Line 273: Replace the assert guarding runtime.group_config with an explicit
None check that raises an appropriate error using the existing diagnostic
message, ensuring the failure occurs even when Python runs with optimizations.
- Around line 485-491: Update the exception handler around cli_obj.run so the
JSON-output branch logs the caught exception, including traceback details,
before emitting the JSON error and exiting. Preserve the existing non-JSON
behavior that re-raises the exception, and do not replace Click output handling
with Flask-specific jsonify.
In `@doozer/tests/cli/test_release_payload.py`:
- Around line 144-146: Strengthen the assertions around cmd in the release
payload test by verifying that no argument with the --from-image-stream option
exists, regardless of its value; retain the existing assertion for the expected
--from-release argument.
- Line 340: Replace the hardcoded /tmp/release-payload value assigned to
self.build_repo.local_dir with a neutral non-filesystem placeholder, preserving
its use in the log message and the test’s behavior.
- Around line 343-442: Add a CliRunner-based test for the
release_payload_rebase_and_build command wrapper, covering KONFLUX_SA_KUBECONFIG
fallback behavior, JSON output via --output json, and exit code 1 when the
command fails. Reuse the existing ReleasePayloadRebaseAndBuildCli test fixtures
and mock the underlying execution so the test verifies wrapper behavior without
performing real rebases or builds.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift-eng/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: e9281ac1-9e04-4ccc-bf9d-1c90268e3e1c
📒 Files selected for processing (4)
doozer/doozerlib/cli/__main__.pydoozer/doozerlib/cli/release_payload.pydoozer/doozerlib/constants.pydoozer/tests/cli/test_release_payload.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@doozer/doozerlib/cli/release_payload.py`:
- Around line 100-107: Update get_component_name so distinct group names
containing "." and "_" cannot produce the same Component name; either validate
and reject ambiguous group names before formatting or encode these separators
bijectively while preserving the existing release-payload prefix and valid-name
requirements.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift-eng/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 21648ec5-0e61-4b6d-ad51-125c66146342
📒 Files selected for processing (2)
doozer/doozerlib/cli/release_payload.pydoozer/tests/cli/test_release_payload.py
🚧 Files skipped from review as they are similar to previous changes (1)
- doozer/tests/cli/test_release_payload.py
9986179 to
04863af
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
doozer/doozerlib/cli/release_payload.py (1)
215-216: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRaise
DoozerFatalErrorinstead ofIOError.Every other failure path in this file raises
DoozerFatalError.IOErroris an alias ofOSErrorand describes an I/O fault, not an invalid state.♻️ Proposed refactor
- if not build_repo.commit_hash: - raise IOError("Release payload repository must have a commit to build. Did you rebase?") + if not build_repo.commit_hash: + raise DoozerFatalError("Release payload repository must have a commit to build. Did you rebase?")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@doozer/doozerlib/cli/release_payload.py` around lines 215 - 216, In the release payload validation around build_repo.commit_hash, replace the IOError exception with DoozerFatalError while preserving the existing message and condition. Ensure the required DoozerFatalError symbol is imported or otherwise available consistently with the other failure paths in this file.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@doozer/doozerlib/cli/release_payload.py`:
- Line 107: Update get_component_name to lowercase the generated release-payload
name before returning it, while preserving the existing dot and underscore
replacement behavior so uppercase group names produce RFC 1123-compatible
Component names.
---
Nitpick comments:
In `@doozer/doozerlib/cli/release_payload.py`:
- Around line 215-216: In the release payload validation around
build_repo.commit_hash, replace the IOError exception with DoozerFatalError
while preserving the existing message and condition. Ensure the required
DoozerFatalError symbol is imported or otherwise available consistently with the
other failure paths in this file.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift-eng/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ffc758d9-cebb-4001-88a1-823992602d90
📒 Files selected for processing (1)
doozer/doozerlib/cli/release_payload.py
| Application (Konflux builds all architectures as one multi-arch manifest list from | ||
| a single PipelineRun per group/assembly), e.g. `release-payload-openshift-4-21`. | ||
| """ | ||
| return f"release-payload-{group}".replace(".", "-").replace("_", "-") |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Lowercase the Component name.
Konflux resource names must match RFC 1123. get_component_name does not lowercase, while the generateName slug at Line 244 does. A group name with an uppercase character produces an invalid Component name and the Konflux API rejects the create call.
🛠️ Proposed fix
- return f"release-payload-{group}".replace(".", "-").replace("_", "-")
+ return f"release-payload-{group}".replace(".", "-").replace("_", "-").lower()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return f"release-payload-{group}".replace(".", "-").replace("_", "-") | |
| return f"release-payload-{group}".replace(".", "-").replace("_", "-").lower() |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@doozer/doozerlib/cli/release_payload.py` at line 107, Update
get_component_name to lowercase the generated release-payload name before
returning it, while preserving the existing dot and underscore replacement
behavior so uppercase group names produce RFC 1123-compatible Component names.
ace248e to
2f1c948
Compare
Adds a new doozer CLI command that generates release payload manifests via `oc adm release new --to-dir`, writes a Dockerfile layering those manifests onto the cluster-version-operator image, pushes the result to openshift-priv/ocp-release-payloads, and triggers a Konflux build of the release payload image. Reuses BuildRepo for git operations and KonfluxClient for Application/Component/PipelineRun management. Co-authored-by: Cursor <cursoragent@cursor.com> rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
2f1c948 to
23a0db5
Compare
Match the format produced by `oc adm release new`: use a two-stage Dockerfile (FROM <cvo> AS cvo → FROM scratch + COPY --from=cvo) and add the io.openshift.release / io.openshift.release.base-image-digest labels so the image is recognized by `oc adm release info`. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
The label value should be the version string (e.g. 4.22.9), not the full version-release composite, matching what oc adm release new sets. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
2 participants
Summary
Implements the doozer rebase+build flow described in ART-21775 (part of the ART-14237 epic to build named release payloads in Konflux instead of running
oc adm release new --to-imagedirectly on buildvm).Adds a new
doozer beta:release-payload:rebase-and-buildcommand that:oc adm release new --to-dirto snapshot the release manifests already populated in the group's build-sync imagestream (imagestream name/namespace derived automatically from--group/--assembly, or optionally sourced from an existing release pullspec via--from-release)cluster-version-operatorpullspec from the generatedimage-referencesmanifestFROM <cvo-pullspec>+COPY release-manifests/ /release-manifests/)openshift-priv/ocp-release-payloadson a per-group/assembly branch, reusingBuildRepoPipelineRunviaKonfluxClient, reusing existing Konflux build infrastructure--push(git push + Konflux build vs. local-only rebase),--dry-run(skip actual git pushes / Konflux API calls), and--output jsonfor machine-parseable resultsKonflux builds a single multi-arch manifest list per PipelineRun, so this command determines all supported architectures from the group config (
runtime.get_global_konflux_arches()) and passes them asbuilding_arches. The--archflag only selects which brew-arch imagestream to source manifests from (e.g.ocpvsocp-s390x).Test plan
doozer/tests/cli/test_release_payload.pycovering naming helpers, imagestream resolution, manifest generation (success + error cases), rebase (Dockerfile/commit), build (Konflux API + outcome handling), and the top-levelrun()flow (--push/--dry-runinteractions, error propagation)uv run pytest doozer/tests/cli/test_release_payload.py-- 21 passeduv run pytest doozer/tests/-- full doozer suite passes (1451 passed, 15 skipped)uv run ruff check/uv run ruff format --checkpass on all new/modified filesSummary by CodeRabbit
New Features
Tests